HDFS-16704. Datanode return empty response instead of NPE for GetVolumeInfo during restarting by ZanderXu · Pull Request #4661 · apache/hadoop

ZanderXu · 2022-07-30T03:40:26Z

Description of PR

During datanode starting, I found some NPE in logs:

Caused by: java.lang.NullPointerException: Storage not yet initialized
    at org.apache.hadoop.thirdparty.com.google.common.base.Preconditions.checkNotNull(Preconditions.java:899)
    at org.apache.hadoop.hdfs.server.datanode.DataNode.getVolumeInfo(DataNode.java:3533)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at sun.reflect.misc.Trampoline.invoke(MethodUtil.java:72)
    at sun.reflect.GeneratedMethodAccessor7.invoke(Unknown Source)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at sun.reflect.misc.MethodUtil.invoke(MethodUtil.java:276)
    at com.sun.jmx.mbeanserver.ConvertingMethod.invokeWithOpenReturn(ConvertingMethod.java:193)
    at com.sun.jmx.mbeanserver.ConvertingMethod.invokeWithOpenReturn(ConvertingMethod.java:175)

Because the storage of datanode not yet initialized when we trying to get metrics of datanode, and related code as below:

@Override // DataNodeMXBean
public String getVolumeInfo() {
  Preconditions.checkNotNull(data, "Storage not yet initialized");
  return JSON.toString(data.getVolumeInfoMap());
}

The logic is ok, but I feel that the more reasonable logic should be return an empty response instead of NPE, because InfoServer will be started before initBlockPool.

hadoop-yetus · 2022-07-30T09:29:23Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 35s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 1s		codespell was not available.
+0 🆗	detsecrets	0m 1s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	38m 3s		trunk passed
+1 💚	compile	1m 44s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	compile	1m 37s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 25s		trunk passed
+1 💚	mvnsite	1m 45s		trunk passed
+1 💚	javadoc	1m 26s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 45s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 38s		trunk passed
+1 💚	shadedclient	23m 9s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 23s		the patch passed
+1 💚	compile	1m 27s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javac	1m 27s		the patch passed
+1 💚	compile	1m 21s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 21s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 0s		the patch passed
+1 💚	mvnsite	1m 25s		the patch passed
+1 💚	javadoc	0m 59s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 32s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 24s		the patch passed
+1 💚	shadedclient	22m 51s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	237m 31s		hadoop-hdfs in the patch passed.
+1 💚	asflicense	1m 15s		The patch does not generate ASF License warnings.
		347m 38s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4661/1/artifact/out/Dockerfile
GITHUB PR	#4661
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux 776d4764cb81 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / 7ce6d940f9e0112b441a66bc6948e8895db0330e
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4661/1/testReport/
Max. process+thread count	3331 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4661/1/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

ayushtkn

I think this a behaviour change, and guess governed by two policies, One is change of public API behaviour and second change in Metrics values as well. Earlier when the storage wasn't initialised the call was failing now it will be returning an empty string. So, that is a behaviour change and can be confused with a state when data.getVolumeInfoMap() is empty.

Extra Stuff: This log line just below is wrong, incorrect placeholder, can fix in a separate jira if interested. :-)

hadoop/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java

Line 3645 in 123d1aa

LOG.debug("Reading diskbalancer Status failed. ex:{}", ex);

ZanderXu · 2022-08-02T02:31:56Z

@ayushtkn Thank you very much for helping me review it.

Yes, you are right, we should return an empty value for this case. How about change it same with getDiskBalancerStatus, such as:

@Override // DataNodeMXBean
  public String getVolumeInfo() {
    if (data == null) {
      LOG.debug("Storage not yet initialized.");
      return ""; // it's different with JSON.toString(new HashMap<String, Object>())
    }
    return JSON.toString(data.getVolumeInfoMap());
  }

Developers or SREs are very sensitive to NPE, so I feel we shouldn't out put it in this expected situation.

Extra Stuff: This log line just below is wrong, incorrect placeholder, can fix in a separate jira if interested. :-)

Copy, sir. I will fix it in a separate Jira.

ayushtkn

The current change makes sense to me.
If no test failures. Changes LGTM.

hadoop-yetus · 2022-08-02T19:13:56Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 38s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	39m 45s		trunk passed
+1 💚	compile	1m 39s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	compile	1m 27s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 21s		trunk passed
+1 💚	mvnsite	1m 33s		trunk passed
+1 💚	javadoc	1m 13s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 40s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 47s		trunk passed
+1 💚	shadedclient	23m 53s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 19s		the patch passed
+1 💚	compile	1m 22s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javac	1m 22s		the patch passed
+1 💚	compile	1m 15s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 15s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 0s		the patch passed
+1 💚	mvnsite	1m 28s		the patch passed
+1 💚	javadoc	0m 58s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 24s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 38s		the patch passed
+1 💚	shadedclient	25m 31s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	236m 38s		hadoop-hdfs in the patch passed.
+1 💚	asflicense	1m 15s		The patch does not generate ASF License warnings.
		350m 10s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4661/2/artifact/out/Dockerfile
GITHUB PR	#4661
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux 23bc297674f0 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / 69287015aa98557ef950041a8a4defe7613417dc
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4661/2/testReport/
Max. process+thread count	3058 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4661/2/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

Hexiaoqiao

LGTM. +1 from my side.

Hexiaoqiao · 2022-08-03T13:27:05Z

@ZanderXu Thanks for your contribution. IIRC, there is also some other precondition checks before initialized, Would you mind to give another check? such as org.apache.hadoop.hdfs.server.datanode.DataNode#initReplicaRecovery or replicaHandler = datanode.data.createRbw(storageType, storageId, block, allowLazyPersist); (which datanode.data is null before DataNode initialized) at here.

ZanderXu · 2022-08-03T16:42:17Z

@Hexiaoqiao Thanks for your review.

Would you mind to give another check?

Sure, I will check them. BTW, If it's ok, can I fix them in a new PR? I hope this PR is just used to fix NPE in getVolumeInfo().

ayushtkn · 2022-08-07T21:09:12Z

Are we good here? or Waiting for some more modifications?

ZanderXu · 2022-08-08T00:50:59Z

Yes, we are here, and I have create a new PR-4699 to fix other cases in DataNode.java.

@ayushtkn @Hexiaoqiao Can you help me review them and give me a suggestion that we fix them in two PR or one?

…meInfo during restarting

ZanderXu · 2022-08-15T10:38:31Z

@Hexiaoqiao @ayushtkn Master, ping, please help me merge this PR and review PR-4699? Thanks

Hexiaoqiao · 2022-08-15T12:57:56Z

Committed to trunk. Thanks @ZanderXu for your contribution, Thanks @ayushtkn for your reviews.

hadoop-yetus · 2022-08-15T16:26:03Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 36s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	38m 1s		trunk passed
+1 💚	compile	1m 43s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	compile	1m 38s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 27s		trunk passed
+1 💚	mvnsite	1m 47s		trunk passed
+1 💚	javadoc	1m 24s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 50s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 43s		trunk passed
+1 💚	shadedclient	23m 11s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 23s		the patch passed
+1 💚	compile	1m 27s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javac	1m 27s		the patch passed
+1 💚	compile	1m 20s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 20s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 2s		the patch passed
+1 💚	mvnsite	1m 26s		the patch passed
+1 💚	javadoc	0m 59s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 29s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 25s		the patch passed
+1 💚	shadedclient	22m 43s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	237m 9s		hadoop-hdfs in the patch passed.
+1 💚	asflicense	1m 15s		The patch does not generate ASF License warnings.
		347m 10s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4661/3/artifact/out/Dockerfile
GITHUB PR	#4661
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux f2e8f1a1b377 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `2af6c2b`
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4661/3/testReport/
Max. process+thread count	3385 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4661/3/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

…meInfo during restarting (apache#4661). Contributed by ZanderXu. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>

ayushtkn requested changes Aug 2, 2022

View reviewed changes

ayushtkn approved these changes Aug 2, 2022

View reviewed changes

Hexiaoqiao approved these changes Aug 3, 2022

View reviewed changes

zengqiang.xu added 2 commits August 15, 2022 18:37

HDFS-16704. Datanode return empty response instead of NPE for GetVolu…

ba1eaeb

…meInfo during restarting

HDFS-16704. Modify patch based on comment

2af6c2b

ZanderXu force-pushed the HDFS-16704 branch from 6928701 to 2af6c2b Compare August 15, 2022 10:37

Hexiaoqiao merged commit b1d4af2 into apache:trunk Aug 15, 2022

Conversation

ZanderXu commented Jul 30, 2022

Description of PR

Uh oh!

hadoop-yetus commented Jul 30, 2022

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

ZanderXu commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

hadoop-yetus commented Aug 2, 2022

Uh oh!

Hexiaoqiao left a comment

Choose a reason for hiding this comment

Uh oh!

Hexiaoqiao commented Aug 3, 2022

Uh oh!

ZanderXu commented Aug 3, 2022

Uh oh!

ayushtkn commented Aug 7, 2022

Uh oh!

ZanderXu commented Aug 8, 2022

Uh oh!

ZanderXu commented Aug 15, 2022

Uh oh!

Hexiaoqiao commented Aug 15, 2022

Uh oh!

hadoop-yetus commented Aug 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ZanderXu commented Aug 2, 2022 •

edited

Loading